GH-3667: Close input readers when ParquetRewriter setup fails - #3662
Open
anxkhn wants to merge 1 commit into
Open
GH-3667: Close input readers when ParquetRewriter setup fails#3662anxkhn wants to merge 1 commit into
anxkhn wants to merge 1 commit into
Conversation
Member
|
I would recommend creating an issue for this. Or at least do not mark it as |
anxkhn
changed the title
Contributor
Author
|
Created #3667 and updated the PR title and body to reference it. Thanks for the guidance. |
nastra
reviewed
Jul 22, 2026
ParquetRewriter.getFileReaders opens each input reader into a local list. If opening a later file fails, close the readers already opened before rethrowing so their input streams do not leak. Preserve close failures as suppressed exceptions on the original failure. Add a regression test that verifies the earlier input stream is closed, using AssertJ assertions. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Contributor
Author
|
Rebased onto current master and updated the regression test to use AssertJ assertions as requested. I also narrowed the PR description to the partial-open failure path covered by this change. The targeted regression test passes (4 tests). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
ParquetRewriter.getFileReadersopens aTransParquetFileReaderfor each inputfile into a local list. If opening a later file throws
IOException, it isrethrown as
IllegalArgumentExceptionwithout closing the readers already openedfor the earlier files, so their
SeekableInputStreamhandles leak. A singlecorrupt, missing, or permission-denied file among several valid inputs can
therefore leak the input streams opened before the partial-open failure.
What changes are included in this PR?
In
getFileReaders, close the readers already opened before rethrowing, usingorg.apache.parquet.util.AutoCloseables.close(...)so every reader is releasedand any close failures are aggregated as suppressed exceptions on the original
IllegalArgumentException. This matches the existing cleanup idiom in the module.The change is limited to the
catch (IOException)block; the success path isunchanged.
Are these changes tested?
Yes. A new
ParquetRewriterTestcase(
testInputFileReadersClosedWhenLaterInputFileFailsToOpen) supplies a validinput file (wrapped so it records whether the stream it hands out is closed)
followed by an
InputFilewhosenewStream()throws, asserts that constructingthe
ParquetRewriterthrowsIllegalArgumentException, and asserts the firstreader's stream was closed. It fails on the pre-fix code (the first stream is
left open) and passes with the fix. The full
ParquetRewriterTestclass passes(
Tests run: 96, Failures: 0, Errors: 0).Are there any user-facing changes?
No public API changes. On a partial-open failure while opening input files,
ParquetRewriternow closes the input streams it had already opened.Closes #3667